feat: Introducing StreamingCredentialsProvider for token based authentication #3320

ndyakov · 2025-03-24T14:12:52Z

Implementation Details and Key Changes

New Authentication Package
- Introduced auth package to hold types related to authentication
- Added auth.Credentials interface to facilitate integration with external credential sources
- Implemented basicAuth for username/password credentials
Streaming Credentials Provider
- Introduced StreamingCredentialsProvider interface for dynamic credential updates
- Added support for managed identity scenarios where credentials are retrieved from external sources
- Implemented ReAuthCredentialsListener for handling credential updates
- Added comprehensive test coverage for all authentication components
Documentation Updates
- Added detailed Authentication section to README.md
- Documented all credential provider types and their priority order:
  - Streaming Credentials Provider (highest priority)
  - Context-based Credentials Provider
  - Regular Credentials Provider
  - Username/Password fields (lowest priority)
- Added example of Entra ID (Azure AD) authentication integration
Hooks System Enhancement
- Refactored hooksMixin to propagate hooks to child connections
- Hooks are now triggered prior to the initialization process
- This change addresses reported issues with hook propagation
Testing
- Added unit tests for all authentication components
- Tested credential provider priority order
- Tested streaming credentials provider with mock implementation
- Tested error handling and edge cases

Security Considerations

Important Note: The hooksMixin refactor will now propagate hooks to child connections and trigger them prior to the initialization process. This change may have security implications, particularly regarding the visibility of authentication commands. For example, Redis MONITOR doesn't report AUTH commands, and this change could potentially expose sensitive authentication information through hooks. This should be carefully considered and potentially documented for users implementing custom hooks.

Related Issues

Fixes issues with hook propagation to child connections
- [Bug] The connection object returned by the redis instance call Conn() method loses the Hook #3267
- Copy client hooks to child connections. #2680
Improves documentation for authentication system

…-auth

options.go

elena-kolevska

LGTM but I think the PR title is misleading. From what I understand, this PR doesn't provide the possibility for authentication with a StreamingCredentialsProvider just yet, instead it introduces the necessary interfaces and already prepares the ground by refactoring the basic auth we were using to use the new Credentials interface.

ndyakov · 2025-03-25T12:12:31Z

Reverting this back to Draft, will continue working on it before v9.8.0-beta.2

Introduces the StreamingCredentialsProvider as the CredentialsProvider with the highest priority. TODO: needs to be tested

Change CancelProviderFunc to UnsubscribeFunc

Copilot

Pull Request Overview

This PR introduces a streaming credentials provider mechanism for token‐based authentication, refactoring connection initialization and hook propagation while also updating tests and documentation for authentication.

Introduces new types and interfaces in the auth package for dynamic credential updates.
Refactors core connection initialization to use the streaming provider and re-authentication listener.
Updates tests and docs (including README and examples) to cover new authentication flows and hook propagation improvements.

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tx.go	Moves hooksMixin from Tx struct into baseClient; updates newTx() accordingly.
sentinel.go	Updates onClose hook assignment using wrappedOnClose.
ring_test.go	Enhances pipeline hook tests by skipping connection initialization commands.
redis_test.go	Introduces tests verifying proper priority usage of Streaming, context, regular, and static credentials.
redis.go	Adds functions to support re-authentication on credentials updates; adjusts error wrapping and hook usage.
probabilistic_test.go	Modifies expectations to be more flexible about command lengths.
osscluster*.go	Minor nil-check improvements and injection of streaming authentication options.
options.go	Updates option comments and adds StreamingCredentialsProvider field.
internal_test.go, example_instrumentation_test.go, doctests, command_recorder_test.go	Additions and tweaks to tests and examples for improved instrumentation and command recording support.
auth/*	New auth package files implementing the streaming credentials API and its tests.
README.md	Updates documentation describing the new authentication priority order and provider usage.
.github/wordlist.txt	Updates wordlist with new authentication-related keywords.

Comments suppressed due to low confidence (2)

tx.go:29

Verify that removing hooksMixin from the Tx struct and relying on baseClient.hooksMixin is an intentional design change to ensure that all hook functionality remains effective.

hooksMixin: c.hooksMixin.clone(),

redis.go:441

There appears to be a potential naming inconsistency between 'DisableIdentity' and 'DisableIndentity'. Please confirm that the correct option name is used to avoid confusion.

if !c.opt.DisableIdentity && !c.opt.DisableIndentity {

elena-kolevska

One comment and small question, but in general lgtm

README.md

doctests/lpush_lrange_test.go

bobymicroby

LGTM! Great job on this, the PR looks awesome! I was initially wondering about omitting the hooksMixin when creating the connection for re-authentication (cn := newConn(c.opt, connPool, nil) in reAuthConnection). However, it makes sense as a defensive measure to prevent potential infinite loops if a hook itself indirectly triggered a re-authentication.

Copilot

Pull Request Overview

This PR introduces a new StreamingCredentialsProvider for token‐based authentication while refactoring several internal authentication flows and improving test coverage. Key changes include adding support for streaming credentials (and their dynamic updates), refactoring connection and authentication flows (e.g. re-authentication and onClose handling), and updating documentation and tests to reflect the new authentication methods.

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tx.go	Refactoring Tx initialization – removal of hooksMixin in the struct versus its usage.
sentinel.go	Updates to onClose handling via wrappedOnClose and removal of unused hooksMixin.
ring_test.go	Updated test expectations to accommodate connection initialization commands.
redis_test.go	Added tests to validate credential provider priority and streaming credential updates.
redis.go	Refactored authentication flow, adding streaming and context-based provider support.
osscluster.*, options.go	Enhanced configuration options and documentation for authentication mechanisms.
auth/*	New implementations and tests for StreamingCredentialsProvider and ReAuthCredentialsListener.
README.md	Updated authentication documentation and examples supporting streaming provider usage.
Others	Minor improvements and documentation updates across tests and helpers.

Comments suppressed due to low confidence (2)

tx.go:29

The Tx struct no longer contains a hooksMixin field, yet newTx still attempts to initialize it. Either remove the hooksMixin initialization in newTx or re-add the hooksMixin field to the Tx struct.

hooksMixin: c.hooksMixin.clone(),

redis_test.go:860

[nitpick] Using a fixed sleep duration to wait for credential updates might lead to flaky tests; consider replacing it with a synchronization mechanism (e.g. a WaitGroup or channel) to ensure the update is processed before asserting.

time.Sleep(100 * time.Millisecond)

ndyakov added 2 commits March 18, 2025 11:07

wip

5410adb

Merge remote-tracking branch 'origin/master' into ndyakov/token-based…

9ef438b

…-auth

ndyakov self-assigned this Mar 24, 2025

ndyakov added 2 commits March 24, 2025 16:29

update documentation

df9bfce

add streamingcredentialsprovider in options

140a278

ndyakov requested a review from htemelski March 24, 2025 15:22

ndyakov marked this pull request as ready for review March 24, 2025 15:22

Merge branch 'master' into ndyakov/token-based-auth

d3a25f9

vladvildanov reviewed Mar 24, 2025

View reviewed changes

options.go Outdated Show resolved Hide resolved

ndyakov added 2 commits March 24, 2025 18:11

fix: put back option in pool creation

7f5d87b

add package level comment

fa59cce

vladvildanov previously approved these changes Mar 24, 2025

View reviewed changes

ndyakov mentioned this pull request Mar 24, 2025

Add CredentialsProvider field to UniversalOptions #2927

Open

elena-kolevska reviewed Mar 24, 2025

View reviewed changes

ndyakov marked this pull request as draft March 25, 2025 12:11

ndyakov added 3 commits April 1, 2025 00:27

Merge branch 'master' into ndyakov/token-based-auth

c248425

Merge branch 'master' into ndyakov/token-based-auth

847f1f9

Initial re authentication implementation

40a89c5

Introduces the StreamingCredentialsProvider as the CredentialsProvider with the highest priority. TODO: needs to be tested

ndyakov dismissed vladvildanov’s stale review via 40a89c5 April 15, 2025 10:37

ndyakov and others added 2 commits April 16, 2025 12:12

Change function type name

d0a8c76

Change CancelProviderFunc to UnsubscribeFunc

Merge branch 'master' into ndyakov/token-based-auth

e0c224d

ndyakov marked this pull request as ready for review April 22, 2025 09:05

add tests

44628c5

ndyakov force-pushed the ndyakov/token-based-auth branch from ff04986 to 44628c5 Compare April 22, 2025 12:41

ndyakov added 3 commits April 22, 2025 15:47

fix race in tests

4ab4980

fix example tests

420c4fb

wip, hooks refactor

5fac913

ndyakov force-pushed the ndyakov/token-based-auth branch from 8fd3bd5 to 5fac913 Compare April 22, 2025 18:11

fix build

2a97f2e

Copilot AI reviewed Apr 24, 2025

View reviewed changes

Merge branch 'master' into ndyakov/token-based-auth

cff6b9b

elena-kolevska previously approved these changes Apr 30, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

doctests/lpush_lrange_test.go Show resolved Hide resolved

ndyakov mentioned this pull request May 7, 2025

feat: add lock for hooks.dial #2460

Closed

Merge branch 'master' into ndyakov/token-based-auth

8f05aef

ndyakov dismissed elena-kolevska’s stale review via 8f05aef May 12, 2025 16:08

feat(conn): add close hook on conn

c5054e2

ndyakov force-pushed the ndyakov/token-based-auth branch 3 times, most recently from 79185b7 to 2f1d17e Compare May 13, 2025 20:53

fix(tests): simulate start/stop in mock credentials provider

8b51596

ndyakov force-pushed the ndyakov/token-based-auth branch from 2f1d17e to 8b51596 Compare May 13, 2025 21:31

ndyakov added 2 commits May 14, 2025 10:06

fix(auth): don't double close the conn

b80969f

docs(README): mark streaming credentials provider as experimental

a6a2c9d

ndyakov force-pushed the ndyakov/token-based-auth branch from 4b37fed to 6e58e79 Compare May 14, 2025 07:38

fix(auth): streamline auth err proccess

5228611

ndyakov force-pushed the ndyakov/token-based-auth branch from 6e58e79 to 5228611 Compare May 14, 2025 07:48

bobymicroby previously approved these changes May 14, 2025

View reviewed changes

fix(auth): check err on close conn

3345fd1

ndyakov dismissed bobymicroby’s stale review via 3345fd1 May 14, 2025 08:50

ndyakov mentioned this pull request May 15, 2025

Commands wrapped in a Watch callback are not hooked if client is a cluster #1547

Open

ndyakov and others added 4 commits May 19, 2025 17:36

Merge branch 'master' into ndyakov/token-based-auth

1628b87

Merge branch 'master' into ndyakov/token-based-auth

57584be

chore(entraid): use the repo under redis org

45e5ee9

Merge branch 'master' into ndyakov/token-based-auth

b7ce3cd

ndyakov force-pushed the ndyakov/token-based-auth branch from 24eb2fc to b7ce3cd Compare May 27, 2025 13:06

ndyakov requested a review from Copilot May 27, 2025 13:15

Copilot AI reviewed May 27, 2025

View reviewed changes

htemelski approved these changes May 27, 2025

View reviewed changes

ndyakov merged commit 86d418f into master May 27, 2025
30 of 31 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: Introducing StreamingCredentialsProvider for token based authentication #3320

feat: Introducing StreamingCredentialsProvider for token based authentication #3320

Uh oh!

ndyakov commented Mar 24, 2025 •

edited

Loading

Uh oh!

Uh oh!

elena-kolevska left a comment •

edited

Loading

Uh oh!

ndyakov commented Mar 25, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

elena-kolevska left a comment

Uh oh!

Uh oh!

Uh oh!

bobymicroby left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

feat: Introducing StreamingCredentialsProvider for token based authentication #3320

feat: Introducing StreamingCredentialsProvider for token based authentication #3320

Uh oh!

Conversation

ndyakov commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation Details and Key Changes

Security Considerations

Related Issues

Uh oh!

Uh oh!

elena-kolevska left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndyakov commented Mar 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

elena-kolevska left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bobymicroby left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

ndyakov commented Mar 24, 2025 •

edited

Loading

elena-kolevska left a comment •

edited

Loading